Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (9fa2fb1) to head (1216b5a).
Report is 26 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1548      +/-   ##
==========================================
- Coverage   99.67%   99.64%   -0.04%     
==========================================
  Files          76       76              
  Lines       11234    11115     -119     
==========================================
- Hits        11198    11076     -122     
- Misses         36       39       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I am still unsure whether it is worth keeping the two different ways for computing the dense form of the SymbolicHamiltonian, namely from_terms and from_form.
Therefore I've done a little benchmark on 4 different hamiltonians ("TFIM", "XXZ", "TFIM**2", "XXZ**2") with the numpy backend for qubits from 2 to 10. In particular the number of terms (after expansion) for a given number of qubits for the TFIM**2 (the biggest) is:

{
    "2q": 36,
    "3q": 81,
    "4q": 144,
    "5q": 225,
    "6q": 324,
    "7q": 441,
    "8q": 576,
    "9q": 729,
    "10q": 900,
    "11q": 1089
}

the average runtimes across 10 runs are as follows:

time vs nqubits
time_vs_nqubits.pdf

time vs nterms
time_vs_nterms.pdf

the from_terms has an advantage when the number of terms is "smaller" (less than ~40 let's say), after that the from_form seems to prevail, but, on the long run, for a large number of qubits and terms the from_terms appears to become comparable if not better.

Long story short, if I had to pick only one of the two I'd probably lean towards the from_terms, but I could also internally automatically switch between the two given a pre-defined nterms threshold.

EDIT: a small update on this one: I've done some minor optimization to the _get_symbol_matrix function and cached it, which made the picture clearer. Now from_form appears to constantly outperform from_terms

at this point it is maybe not worth keeping from_terms anymore....

I marked the PR as ready for review, but keep in mind that this

Note: tests on cupy are still failing due to this qiboteam/qibojit#196 .

is still up. I am testing right now whether there I can fix them in qibojit directly (qiboteam/qibojit#199), otherwise I will have to distribute some casts here.

@stavros11 @renatomello may I ask you guys to have a look a this if you have time, I presume you two were the main contributors to the hamiltonian module. There are still some parts of the code that I left in commented because they are up for discussion and I would like to conduct some more tests on a GPU.

Thanks @BrunoLiegiBastonLiegi for the refactor. This part of is admittedly quite messy. I did not have a detailed look in the code (will do soon), just some quick feedback on the first comment.

EDIT 2: I went ahead and provided both Symbols and SymbolicTerm with a backend attribute which is useful whenever you move to the dense form representation. Initially, I thought about making backend an additional input argument to the functions that needed it (similarly to what we do with gates), but this would have meant breaking the api (e.g. symbol.matrix -> symbol.matrix(backend)).

Personally, I don't see this as a big issue, particularly because the old gate.matrix was also converted to gate.matrix(backend) at some point. I guess you could also have a default value so that symbol.matrix() is sufficient. It would still be breaking and of course up to discussion, maybe to be postponed to a major version release (if we follow that). In any case, if not done here, I would open an issue to do it in the next major release,

What it means now though, is that there are cases where the backends of the different pieces may mismatch, thus causing problems. To mitigate this, I decided to forcely set the backend of the symbols here https://github.com/qiboteam/qibo/blob/4d8c9c1211c850a5e740607de943da75fc31f8b2/src/qibo/hamiltonians/terms.py#L176C1-L179C50 which allows for the end user to not care about which backend to construct a symbol with, maintaining in practice the usual API.

particularly to avoid the need for such workarounds.

As part of the refactor, I removed the symbol_map argument of the hamiltonians,

I don't fully remember what's that for, but based on the docstring it was planned to be removed anyway, so it should be fine.

the from_symbolic method and the TrotterHamiltonian.

In principle, these are also kind of "breaking" so we could even just do a breaking release with this PR.